Conversation
|
The latest upstream CI run failed in I reran the unchanged PR head ( There is a plausible sampling race: the test disables displacement recording during wheel input and reenables it after the final animation frames. A range change completed in that interval is discarded. A controlled exercise of the existing probe reproduces zero recorded boundaries in that ordering. This supports a timing-sensitive test failure, though it does not prove the exact frame ordering in the failed CI run. Could a maintainer rerun the failed upstream job at https://github.com/apache/maka/actions/runs/34697088741 ? GitHub rejected my rerun request because it requires repository admin rights. No assertions, retries, or code were changed for this rerun. |
me2seeks
left a comment
There was a problem hiding this comment.
P2 — Restore the historical protocol declaration and remove unrelated hook changes
The Side Conversation parent-status implementation is sound. However, merge commit ed5a813 also changes work that is unrelated to #5230:
packages/runtime-host/protocol-compatible-changes/base64-length-allocation.jsonchanges the existing declaration from epoch147to149, although neither the declared protocol files nor the declared behavior are changed here. The upstream merge parent and currentmainboth contain147; the feature-branch parent contains143.149is introduced only by this merge result.scripts/biome-staged-check.mjsand its test add a rule that skips every staged.patchand.difffile. This is an independent pre-commit behavior change and is absent from both merge parents.
Please restore the declaration to 147 and remove the Biome hook/test change from this PR. If the hook behavior is desired, it needs its own issue, scope, and review.
中文说明
Side Conversation 显示主任务状态的实现本身没有问题。但合并提交 ed5a813 额外把既有 Base64 协议兼容性声明的 epoch 从 147 改成了 149:上游父提交和当前 main 都是 147,功能分支父提交是 143,只有这次合并结果出现 149。同时,它还引入了所有 .patch/.diff 文件跳过 staged Biome 检查的独立行为和测试。
请将 epoch 恢复为 147,并从本 PR 移除 Biome 钩子及测试改动;如需该钩子行为,请单独建有明确范围的 PR。
ed5a813 to
35bde23
Compare
Project canonical pending interaction kinds and root execution into the companion panel, with settled-turn history fallback and lifecycle guards. Reuse the parent conversation controls and cover navigation, reconnect, and browser focus behavior. Refs apache#5230 Generated-by: Grok Build (grok-4.6, xhigh) Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Reuse the continuity fixture with idle session state and the matching host epoch so parent execution observation receives canonical interaction data. Generated-by: Grok Build (grok-4.6, xhigh)
35bde23 to
fa16fcf
Compare
Preserve parent conversation focus and the current Work Board callbacks and regression tests. Regenerate the surface inventory from the merged tree. Generated-by: OpenAI Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Technical review: NO-GO (1 P2).
This change adds a parent-task execution projection and Side Conversation banner for waiting, running, terminal, and unavailable states. The data minimization and session scoping are sound, and the previous unrelated protocol/Biome diff is no longer present. One recovery path remains incomplete: a terminal observation error leaves the open Side Conversation permanently unsubscribed, so a recovered Host cannot restore the parent status without a session switch or reopening the panel.
Validation on caa2ba2580cc9caf8feb1f72733b2e7ea70426ea: build, full typecheck/lint/format, ASF headers, renderer architecture 112/112, E2E budget 38, focused tests 213/213, Storybook build and render smoke 361 stories / 390 theme renders, and hosted test passed. Desktop full suite completed with 2496 passes and 8 cancellations isolated to the unchanged MCP OAuth deadline suite, which reproduces when run alone. The head is mergeable with current main 4a42aaeab4a7435a84c7d512e81b4d803f9ee0aa.
The new tests cover normal reconnection through an existing callback, but not the production terminal-error contract where main removes the observer registration. Please add bounded resubscription and a regression requiring a second subscription after onSeedError. I did not validate packaged Windows/macOS behavior or a prolonged real Host outage.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| sessionId, | ||
| () => undefined, | ||
| undefined, | ||
| () => { |
There was a problem hiding this comment.
[P2] Resubscribe after an observation error
This callback only marks the projection unavailable, but an observation error is terminal for this registration. Main sends host_observation_error and immediately closes the observer state, which removes the observer ID; an initial sessions:observe rejection likewise leaves no active registration. Because this effect's dependencies do not change, a recovered Host can no longer deliver another seed, so every open Side Conversation keeps showing “Parent task status is unavailable” until the user switches Sessions or closes and reopens the side chat.
A compiled-hook probe where the first subscription called onSeedError and the second would seed a running Turn remained at subscriptionCount=1,status=unavailable after 250 ms. The current reconnect test injects a later projection through the same callback, so it does not cover this terminal path. Please cancel and retry the subscription with bounded backoff, as the App Shell observer does, and add a regression that requires a second subscribeEvents call after onSeedError.
Replace terminally failed observer registrations using capped exponential backoff, cancel retries on cleanup, and reject late callbacks. Cover synchronous failures, retry reset, session changes and history refresh with regression tests. Generated-by: OpenAI Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Technical review: GO. I found no remaining P0-P3 issues on this head.
The previous observation-recovery P2 is fixed. A terminal observation error now invalidates and releases the failed registration, retries with bounded exponential backoff, fences late callbacks by attempt, cancels pending retries on Session change or unmount, and rereads settled history after the replacement registration. The new regression coverage exercises synchronous failure, delay capping/reset, stale callbacks, cleanup, and stale history reads through the production hook contract.
Validation on 79792f04307d505d8eb4c03bf7653758151f86a8: build:test, full typecheck/lint/format, ASF headers, renderer architecture 112/112, E2E budget 38, focused production-path tests 176/176, UI 464/464, renderer and Storybook builds, Storybook render smoke 361 stories / 390 theme renders, and the full Electron E2E suite 38/38. The exact Side Chat follow-up scenario that failed once in hosted CI also passed 10/10 additional local runs. The Desktop suite completed with 2500 passes and 8 cancellations isolated to the unchanged MCP OAuth deadline tests, which reproduce when run alone.
The hosted test check is still red from that single unreproduced Side Chat E2E failure, so it should be rerun successfully before merge. The head merges cleanly with current main ea990cab7ffa768dc1a574b539f73a83f8faabb8. I did not validate native Windows/macOS behavior or a prolonged real Host outage.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Merge apache/maka main 4410c3a. Conflicts resolved: - workbar-controller.test.ts: keep the parent focus tests beside the upstream live browser view activation test. - session-workbar.stories.tsx: adopt the upstream SessionUsageSummary rename and keep the SessionExecutionProjection import. - docs/astryx-surface-file-inventory.md: regenerate the inventory. Generated-by: Pi (DeepSeek V4.1 Flash)
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at head fc61965181. The observation architecture is right: one sessions:observe per WorkbarSurface riding the existing registry/observer seam (no parallel subscription), pendingInteractionKinds added to the existing projection, fan-out to all side panels, stale reads fenced by historyEpoch, retry lifecycle verified (failed registration is unsubscribed before backoff retry, 100ms→2s capped, reset on seed; callbacks from failed registrations ignored; session deletion does not cause a retry storm). Pending kinds → live rootTurn → settled listTurns precedence is correct, idle correctly renders nothing, and only interaction kinds cross the wire — no prompt content leaks.
One heads-up on the still-open review thread: the resubscribe-after-observation-error concern is addressed at this head — the hook cancels the failed registration and resubscribes with bounded backoff, with direct tests. The thread just isn't marked resolved.
Findings: one P2 on the WorkHub surface (inline), plus small items:
- P3 — a failed
listTurnsread never retries (inline):latestTurnRead:'failed'maps tounavailableand nothing schedules a reread until a projection migration bumpshistoryEpoch, so a transient read failure can leave the warning banner up indefinitely on a settled parent. - P3 — "status unavailable" warning flashes on every mount (inline):
execution === undefined(pre-seed) maps tounavailable, so every side-chat open briefly renders a failure banner that isn't one. - P3 —
hasSideChatgates on the global panel topology, not visible tabs (workbar-surface.tsx:434-437):positionedTabsretains side-chat tabs belonging to other sessions, so the hook subscribes whenever any side-chat tab exists anywhere — an extra registration on the viewed session with no visible panel for it. Derive fromvisiblePanels(or the already-filteredquotes). - P3 — a second full observation: the hook's
sessions:observeruns parallel to App Shell's existing one, and each mount/retry re-seeds the whole active transcript over IPC only to drop every event (() => undefinedhandler). Either port the App-Shell projection into the workbar or add an execution-only observe option. - P3 — accessibility: the notice has no
role="status"/aria-live, so running→waiting→failed transitions aren't announced (chat-composer-region.tsx:312has the precedent). Alsopermissionkind maps towaiting_approval, but no respondable permission control exists on the desktop surface — the action then falls back to focusing the composer, which is fine, but the copy implies a button that isn't there.
Manual checks a reviewer cannot do from the diff: first-paint flash, the focus jump landing on the right control in both AppShell and WorkHub, banner wrap at the 320px panel floor, and reconnect → correct terminal status. The story SideChatParentWaitingApproval exists — please confirm light/dark captures ship in the body.
AI assistance: delegated reviewers audited the host observation path and the renderer lifecycle independently; I verified the load-bearing claims against the head source myself.
中文
Head fc61965181。观测架构是对的:每个 WorkbarSurface 一条 sessions:observe,走既有 registry/observer 缝(没有并行订阅),pendingInteractionKinds 加在既有投影上,所有 side panel 共享一份;stale read 有 historyEpoch 防;重试生命周期核验过(失败注册先退订再退避重订阅,100ms→2s 封顶,seed 后重置,失败注册的回调被忽略,会话删除不会引发重试风暴)。优先级 待处理 kind → 活跃 rootTurn → settled listTurns 正确;idle 不渲染;wire 上只有 kind 没有内容泄漏。
那个还开着的评审 thread(观测错误后重订阅)在当前 head 其实已经修了——hook 会取消失败注册并按界退避重订阅,有直接测试,只是没人点 resolve。
发现:一个 P2 在行内(WorkHub 表面上"前往主对话"按钮静默失效),小项:P3 listTurns 读失败不重试导致警告常驻;P3 挂载瞬间闪 "unavailable" 横幅(未 seed 时应渲染 null 而非警告);P3 hasSideChat 按全局面板拓扑判定会多挂一条观察;P3 第二条完整观察每次挂载把整段活跃 transcript 过 IPC 推来又丢弃;P3 notice 缺 role="status",且 permission kind 在桌面端没有可应答控件。
| } | ||
|
|
||
| const PARENT_INTERACTION_FOCUSABLE = | ||
| '.maka-composer-interaction-slot button, .maka-composer-interaction-slot [href], .maka-composer-interaction-slot input, .maka-composer-interaction-slot textarea, .maka-composer-interaction-slot [contenteditable="true"]'; |
There was a problem hiding this comment.
P2 — "Go to parent conversation" silently does nothing on the WorkHub surface, exactly when it matters. Both selectors require the target inside .maka-composer-interaction-slot or a visible .maka-composer, but WorkHub renders FormInteractionPrompt/UserQuestionPrompt directly inside .workHubComposerSurface (workhub-root.tsx:311-314) and hides its .maka-composer while a question/form is active (:315 hidden) — closest('[hidden]') then excludes it, so there is no focus target and the click is a silent no-op. The notice says the parent is waiting for input and the button does nothing. Smallest fix: search from the notice's own .maka-detail-with-artifacts ancestor instead of a global .mainColumn, and give prompt containers a semantic contract attribute (e.g. data-maka-contract="session-interaction") both hosts share, rather than depending on composer internals.
| readonly latestTurnRead: ParentTaskLatestTurnRead; | ||
| }): ParentTaskStatusKind { | ||
| const execution = input.execution; | ||
| if (!execution?.available) return 'unavailable'; |
There was a problem hiding this comment.
P3 — pre-seed undefined is not the same as unavailable. Before the first projection arrives, execution is undefined and this maps to unavailable, rendering a warning banner for one IPC round-trip on every mount/session switch — a failure claim that isn't one. Return null while execution === undefined; real losses still arrive as available: false.
| } | ||
|
|
||
| const latest = input.latestTurnRead; | ||
| if (latest.status === 'pending' || latest.status === 'failed') return 'unavailable'; |
There was a problem hiding this comment.
P3 — a failed listTurns read never retries. latestTurnRead:'failed' maps to unavailable, and shouldInvalidateHistoryRead only refires on availability loss or live-root transitions — a settled/idle parent may produce neither, so one transient persistence_failed leaves the warning up indefinitely. Treat available && !rootTurn && latestTurnRead.status === 'failed' as a refresh trigger in applyExecution (one condition + epoch bump; frames on a settled session are rare, so it stays bounded).
me2seeks
left a comment
There was a problem hiding this comment.
Reviewed exact head fc619651811a38cd80ac2e4694d00dabaf07fa2b. The unrelated protocol-epoch and staged-Biome changes from my earlier review are gone, and the observation/resubscription architecture is sound. One user-facing path remains blocking: on the WorkHub surface, "Open parent conversation" cannot focus the question/form that caused the parent to wait. I also recorded two non-blocking recovery/display gaps inline.
| } | ||
|
|
||
| const PARENT_INTERACTION_FOCUSABLE = | ||
| '.maka-composer-interaction-slot button, .maka-composer-interaction-slot [href], .maka-composer-interaction-slot input, .maka-composer-interaction-slot textarea, .maka-composer-interaction-slot [contenteditable="true"]'; |
There was a problem hiding this comment.
P2 — "Open parent conversation" is a silent no-op on the WorkHub surface. This search only recognizes controls below .maka-composer-interaction-slot, then falls back to a visible .maka-composer. WorkHub renders FormInteractionPrompt and UserQuestionPrompt directly under .workHubComposerSurface, while .workHubComposerContent (and therefore its composer) is hidden whenever either prompt is active. In the exact state where this notice says the parent is waiting for input, neither selector can return a usable target, so clicking the action does nothing. The current focus tests construct the App Shell wrapper and do not exercise WorkHub's real DOM.
| readonly latestTurnRead: ParentTaskLatestTurnRead; | ||
| }): ParentTaskStatusKind { | ||
| const execution = input.execution; | ||
| if (!execution?.available) return 'unavailable'; |
There was a problem hiding this comment.
P3 — The normal pre-seed state is rendered as an outage. execution is undefined before the first observation seed arrives, but this maps it to unavailable, so every mount/session switch can briefly show "Parent task status is unavailable" even though no read has failed. A missing pre-seed projection should render no status; actual observation loss already arrives as available: false.
| } | ||
|
|
||
| const latest = input.latestTurnRead; | ||
| if (latest.status === 'pending' || latest.status === 'failed') return 'unavailable'; |
There was a problem hiding this comment.
P3 — A transient settled-history read failure can leave the unavailable banner indefinitely. After listTurns fails, latestTurnRead: 'failed' maps to unavailable, but the invalidation logic only retries on availability loss or live-root transitions. An idle/settled parent may produce neither, so there is no automatic reread until some unrelated projection change.
Summary
Side Conversations now show when the parent task needs input or approval, is running, or most recently completed, failed, or was interrupted. The notice uses canonical Host execution and pending interaction kinds, with settled Turn records for sessions opened after a run ends. An idle session is not reported as completed.
All side panels share one parent observation. Terminal observation errors cancel the failed registration and resubscribe with exponential backoff from 100 ms to a 2-second cap, reset on a successful seed. Navigation and unmount cancel retries; callbacks from failed registrations are ignored. Navigation and reconnect invalidate stale reads, including lifecycle notifications delivered in one React batch. The action focuses the existing parent interaction or composer; responses still use the existing handlers.
Fixes #5230. Refs #4331 (parent-status item only).
Verification
Validated merge commit
fc619651811a38cd80ac2e4694d00dabaf07fa2bagainst upstream4410c3a2d19d20cfdc6b7815bdd2d72d33a7460f:side-chat-followupsE2E passed both alone and in the full run. Upstream99098aafbadds submission-readiness waits at the affected send sites; those changes are included unchanged by the merge. No assertions, timeouts or retry counts were relaxed for this PR.new-task-reload.spec.tsfails while archiving a Session with a live derived effect. The same failure reproduces on an upstream-only4410c3a2dtree; it remains unfixed here. These macOS results do not establish that the hosted Linux check passes.SessionUsageSummaryrename, and regenerates the surface inventory. The PR still changes the same 27 files relative to upstream. Historical Base64 protocol declarations and the Biome hook/test match upstream.Storybook, 1280×900, light/default. Before is the existing SideChat story from unmodified base
83aa12a29; after adds the parent sandbox approval scenario in the same Workbar frame. These are component scenarios, not live Electron screenshots.AI use
Tool(s) and scope: Grok Build (
grok-4.6,xhigh) implemented the feature, regression tests and Storybook scenario. OpenAI Codex reviewed the changes, independently reproduced a reconnect bug, verified the fixes and prepared this PR on behalf of @hbw00111. The feature commit includes bothGenerated-bytrailers; OpenAI Codex also resolved the main conflicts, fixed terminal observer recovery, added regression coverage, and ran the revision checks listed above. Native Codex agents independently inspected the subscription contract and reviewed the fix; this is not independent human approval. Pi (Cline Pass DeepSeek V4.1 Flash, high) resolved the current upstream merge conflicts, ran local validation and the upstream E2E comparison; OpenAI Codex planned and reviewed this revision and independently reran the key regressions. The merge commit includes aGenerated-by: Pi (DeepSeek V4.1 Flash)trailer.Checklist
Does this PR entail a change in behavior?